-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Conversation
@ivovandongen, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @tmpsantos and @mollymerp to be potential reviewers. |
b997865
to
a8d0eee
Compare
a8d0eee
to
97ebe13
Compare
@tmpsantos Could you review please (assuming the ci completes)? I'm not very happy with the name |
How about |
With these changes, is it still necessary for the default |
I like this one - though we should be aware that the file URI scheme can also point to files on a network location. |
👍 |
97ebe13
to
0c836e6
Compare
Seems popular :) I've changed this.
I assumed we need to leave it in there now for backwards compatibility.
Removed |
@@ -19,6 +20,12 @@ bool isAssetURL(const std::string& url) { | |||
return std::equal(assetProtocol.begin(), assetProtocol.end(), url.begin()); | |||
} | |||
|
|||
const std::string fileProtocol = "file://"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better avoid this. See for alternatives: #6455
|
||
namespace { | ||
|
||
std::string toAbsoluteUrl(const std::string& fileName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: we use URL on the internal bits rather then Url.
std::string toAbsoluteUrl(const std::string& fileName) { | ||
char buff[PATH_MAX + 1]; | ||
char* cwd = getcwd( buff, PATH_MAX + 1 ); | ||
return "file:://" + std::string(cwd) + "/test/fixtures/storage/assets/" + fileName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
://
public: | ||
void request(const std::string& url, FileSource::Callback callback) { | ||
//Cut off the protocol | ||
std::string path = mbgl::util::percentDecode(url.substr(7)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this like #6455 with the size a global together with the scheme variable.
loop.run(); | ||
} | ||
|
||
TEST(LocalFileSource, URLEncoding) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake we are no missing anything, can you make a test with a extremely large file name, like memset(filename, 'x', PATH_MAX);
Wondering what would happen if you try to read something you don't have permission to (like /root/foo.txt
).
a28508a
to
4427633
Compare
@tmpsantos Thanks for the review. I've cleared up all items.
It returns |
4427633
to
d0755b6
Compare
|
||
namespace mbgl { | ||
|
||
const char* protocol = "file://"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could go to the anonymous namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Changed it.
d0755b6
to
9c3cf32
Compare
#include <sys/stat.h> | ||
#include <unistd.h> | ||
|
||
const char* protocol = "file://"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
namespace {
const char* protocol = "file://";
const std::size_t protocolLength = 7;
} // namespace
True, removing that support might (?) break asset: assets that have been cached, I suppose, and it’s pretty harmless. |
9c3cf32
to
983d359
Compare
983d359
to
39dfb4d
Compare
@tmpsantos I fixed the valgrind errors. If all else is good, could you press the review button? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for [core]
I just wanted to say to @ivovandongen, thank you! Oh, and also @1ec5 and the rest of the mapbox team as well 👍 |
cc: @cammace this looks like a good candidate for a new sample, or a required update to https://www.mapbox.com/android-sdk/examples/custom-raster/. |
I am using ios SDK 3.4.0-beta.6 and am not able to use the file:// style to load my style JSON or in the tiles section of my sources in the style.json. When I use asset:// for the style it loads, but neither asset nor file seem to work within the styles section. |
It should be possible to use an absolute file: URL as an MGLMapView’s style URL. But I’m not sure if it’s supposed to be possible to use a file: URL inside the JSON file itself. In any event, this closed PR likely isn’t being watched by folks who know the answer for sure, so please open a new issue, ideally with more details about your style JSON. |
Fixes #6273